switch to using `scan_data` version of scan for `execute()` as well #265

nicklan · 2024-06-21T22:34:33Z

Previously we had two independent code paths for interacting with tables via a scan, execute() or scan_data().

scan_data is what most engines will use, and is a bit more complex in that it evaluates expressions over the returned add files. This meant that bugs like #261 could happen because our tests used execute which didn't catch the issue.

This PR makes execute use scan_data under the hood. It's a bit more complex, but now we won't need to maintain two code paths.

Until #271 merges, this PR will fail tests, because nullable columns are not filled in as expected.

nicklan · 2024-06-26T23:03:33Z

kernel/src/scan/data_skipping.rs

@@ -242,7 +243,8 @@ impl DataSkippingFilter {
        // 3. The selection evaluator does DISTINCT(col(predicate), 'false') to produce true (= keep) when
        //    the predicate is true/null and false (= skip) when the predicate is false.
        let select_stats_evaluator = engine.get_expression_handler().get_evaluator(
-            stats_schema.clone(),
+            // safety: kernel is very broken if we don't have the schema for Add actions
+            get_log_schema().project(&[ADD_NAME]).unwrap(),


This was incorrectly passing stats_schema as the input schema before. the input to this evaluator is the raw log data, so we want to add schema. it only worked before because we were ignoring the input schema

Not sure I fully understand -- the Add schema has stats as a string (which happens to be a JSON object literal), so it would seem difficult to do data skipping over that? Or is the needed string-to-json parsing already injected somewhere else?

This is the input schema. This evaluator is called on the raw data read from the log, so we're asking for it to get add.stats out of the Add. STATS_EXPR is just Expr::column("add.stats"), and note the output is just a String.

scarman-db

LGMT

kernel/src/engine/arrow_expression.rs

kernel/src/scan/mod.rs

hntd187

LGMT but on the correct account this time

scovich · 2024-06-27T18:35:37Z

kernel/src/scan/mod.rs

+            scan_files =
+                state::visit_scan_files(data.as_ref(), &vec, scan_files, scan_data_callback)?;


remind me why we pass and return scan_files, instead of just passing a &mut scan_files?
(I have vague memories we discussed it at some point in the past, but I forgot the details)

Inside visit_scan_files we construct a ScanFileVisitor which needs to store this context. Having it take ownership simplifies things significantly there (especially for FFI).

LMK if that's enough context, I can explain more deeply if needed.

Aside: If this were a performance sensitive loop, passing and returning by value would defeat return value optimizations and cause object copying that the outparam (&mut) approach would not. But this is once per data chunk, so we're probably fine.

that's true. as you say, this isn't in a hot loop so I think it's okay.

scovich · 2024-06-27T18:48:29Z

kernel/src/scan/data_skipping.rs

@@ -242,7 +243,8 @@ impl DataSkippingFilter {
        // 3. The selection evaluator does DISTINCT(col(predicate), 'false') to produce true (= keep) when
        //    the predicate is true/null and false (= skip) when the predicate is false.
        let select_stats_evaluator = engine.get_expression_handler().get_evaluator(
-            stats_schema.clone(),
+            // safety: kernel is very broken if we don't have the schema for Add actions
+            get_log_schema().project(&[ADD_NAME]).unwrap(),


Not sure I fully understand -- the Add schema has stats as a string (which happens to be a JSON object literal), so it would seem difficult to do data skipping over that? Or is the needed string-to-json parsing already injected somewhere else?

kernel/src/scan/mod.rs

This reverts commit 48f834b.

scovich

Nice work! Only nits left at this point.

scovich · 2024-07-15T20:43:39Z

kernel/src/scan/mod.rs

-                dv_mask = rest;
-                results.push(scan_result);
-            }
+        let global_state = Arc::new(self.global_scan_state());


I don't think this needs to be an Arc at all any more? The only use of global_state is a call to clone, and the resulting gs is passed by shared reference to a helper method?

(I'm actually a bit surprised that &gs even compiles, since the method wants a &GlobalState, not &Arc<GlobalState> -- I guess that's impl Deref and/or impl AsRef at work?)

It's AsRef at work. https://doc.rust-lang.org/src/alloc/sync.rs.html#3611

kernel/src/scan/mod.rs

scovich · 2024-07-15T20:48:42Z

kernel/src/scan/mod.rs

+            scan_files =
+                state::visit_scan_files(data.as_ref(), &vec, scan_files, scan_data_callback)?;


Aside: If this were a performance sensitive loop, passing and returning by value would defeat return value optimizations and cause object copying that the outparam (&mut) approach would not. But this is once per data chunk, so we're probably fine.

kernel/src/scan/mod.rs

nicklan

thanks!

kernel/src/scan/mod.rs

nicklan · 2024-07-16T00:28:11Z

kernel/src/scan/mod.rs

+            scan_files =
+                state::visit_scan_files(data.as_ref(), &vec, scan_files, scan_data_callback)?;


that's true. as you say, this isn't in a hot loop so I think it's okay.

kernel/src/scan/mod.rs

scovich · 2024-07-16T01:19:16Z

kernel/src/scan/mod.rs

+                    global_state.read_schema.clone(),
+                    None,
+                )?;
+                let gs = global_state.clone();


I don't think this is correct/necessary? We're now cloning the state itself (it used to be an arc). Can we not just pass &global_state at L296 below?

ahh, I now remember why I had it as an Arc. We have to move the read_result into the iter, since we return it, but doing move |read_result| moves any captured variable, and we can't move global_state into an FnMut.

I think the best option is actually to go back to using an Arc, so I will do that.

Would it have also worked to capture a reference, e.g. hoist the initialization of gs outside the iter? Because shared refs are clone+copy?

nicklan requested review from zachschuermann, rtyler, hntd187 and roeap June 25, 2024 21:40

nicklan force-pushed the unify-scan-execute-and-scan-data branch from 6de3046 to 3f99297 Compare June 26, 2024 21:01

nicklan commented Jun 26, 2024

View reviewed changes

scarman-db approved these changes Jun 27, 2024

View reviewed changes

kernel/src/engine/arrow_expression.rs Outdated Show resolved Hide resolved

kernel/src/scan/mod.rs Outdated Show resolved Hide resolved

hntd187 approved these changes Jun 27, 2024

View reviewed changes

scovich reviewed Jun 28, 2024

View reviewed changes

nicklan added 7 commits July 9, 2024 16:02

switch to using scan_data version of scan for execute() as well

2861846

add some comments

3ab4ef1

allow struct.col to be null if missing

328b495

proper input schema for stats evaluator

564db8d

fmt

c8deb48

Revert "allow struct.col to be null if missing"

5b51c4e

This reverts commit 48f834b.

fixes after revert

e993a0d

nicklan force-pushed the unify-scan-execute-and-scan-data branch from 3f99297 to e993a0d Compare July 9, 2024 23:02

nicklan added 2 commits July 9, 2024 16:11

don't take ref to arc

723477c

remove need for crazy turbofish

bf82fcf

nicklan requested review from scovich and sppalkia July 9, 2024 23:48

scovich approved these changes Jul 15, 2024

View reviewed changes

nicklan added 3 commits July 15, 2024 17:22

Merge branch 'main' into unify-scan-execute-and-scan-data

28f6034

put back split_vector fix

1db6f43

apply review comments

edd71bb

nicklan commented Jul 16, 2024

View reviewed changes

scovich reviewed Jul 16, 2024

View reviewed changes

nicklan added 2 commits July 16, 2024 13:12

Merge branch 'main' into unify-scan-execute-and-scan-data

60f70ce

go back to Arc so clone is cheap

5721cf3

nicklan merged commit e8c0226 into delta-io:main Jul 16, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch to using `scan_data` version of scan for `execute()` as well #265

switch to using `scan_data` version of scan for `execute()` as well #265

nicklan commented Jun 21, 2024 •

edited

Loading

nicklan Jun 26, 2024

scovich Jun 27, 2024

nicklan Jul 9, 2024

scarman-db left a comment

hntd187 left a comment

scovich Jun 27, 2024

nicklan Jul 9, 2024

scovich Jul 15, 2024

nicklan Jul 16, 2024

scovich Jun 27, 2024

scovich left a comment

scovich Jul 15, 2024

hntd187 Jul 15, 2024

scovich Jul 15, 2024

nicklan left a comment

nicklan Jul 16, 2024

scovich Jul 16, 2024

nicklan Jul 16, 2024

scovich Jul 17, 2024

		scan_files =
		state::visit_scan_files(data.as_ref(), &vec, scan_files, scan_data_callback)?;

switch to using scan_data version of scan for execute() as well #265

switch to using scan_data version of scan for execute() as well #265

Conversation

nicklan commented Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scarman-db left a comment

Choose a reason for hiding this comment

hntd187 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

switch to using `scan_data` version of scan for `execute()` as well #265

switch to using `scan_data` version of scan for `execute()` as well #265

nicklan commented Jun 21, 2024 •

edited

Loading